Fix phpstan/phpstan#13920: Anonymous class causes "variable might not be defined" issue#5205
Conversation
- Throw points from anonymous class constructor body carried inner method scopes instead of the outer scope, causing variables defined before try blocks to be reported as "might not be defined" in finally blocks - Added replaceScope() method to InternalThrowPoint to allow scope replacement - New regression test in tests/PHPStan/Rules/Variables/data/bug-13920.php Closes phpstan/phpstan#13920
VincentLanglet
force-pushed
the
create-pull-request/patch-oi90985
branch
from
7b4cead to
4afa331
Compare
|
The branch is already up to date with the remote. Everything looks good — tests pass, static analysis passes, the review is approved, and the code is pushed. |
| }, StatementContext::createTopLevel()); | ||
| if ($constructorResult !== null) { | ||
| $throwPoints = array_map(static fn (ThrowPoint $point) => InternalThrowPoint::createFromPublic($point), $constructorResult->getStatementResult()->getThrowPoints()); | ||
| $throwPoints = array_map(static fn (ThrowPoint $point): InternalThrowPoint => InternalThrowPoint::createFromPublic($point)->replaceScope($scope), $constructorResult->getStatementResult()->getThrowPoints()); |
There was a problem hiding this comment.
this seems to be the only call-site where we use InternalThrowPoint::createFromPublic.
instead of adding replaceScope I think we can just add a Scope $scope parameter to InternalThrowPoint::createFromPublic - we don't need to look for backwards compatibility, because InternalThrowPoint is not annotated @api.
There was a problem hiding this comment.
Done. I addressed staabm's review by:
- Adding an optional
?MutatingScope $scope = nullparameter toInternalThrowPoint::createFromPublic() - Removing the
replaceScope()method - Updating the call site in
NewHandler.phpto pass$scopedirectly tocreateFromPublic()
Tests and static analysis both pass.
…f replaceScope Move the scope replacement into createFromPublic() as an optional parameter, removing the separate replaceScope() method as suggested in code review. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
src/Analyser/InternalThrowPoint.php
Outdated
| } | ||
|
|
||
| public static function createFromPublic(ThrowPoint $throwPoint): self | ||
| public static function createFromPublic(ThrowPoint $throwPoint, ?MutatingScope $scope = null): self |
There was a problem hiding this comment.
the $scope parameter is not optional and cannot be null.
There was a problem hiding this comment.
Already handled — all 11,571 tests passed, and the fix has been committed and pushed.
…f replaceScope Make the $scope parameter required (non-nullable) since it is always provided at the only call site. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The test passes. The PR is already in the approved state with all review feedback addressed. There's nothing more to change — the code correctly uses a required |
3 participants
Summary
When an anonymous class with an explicit constructor calling
parent::__construct()was used inside atryblock, PHPStan incorrectly reported that variables defined before thetryblock "might not be defined" in thefinallyblock.Changes
src/Analyser/ExprHandler/NewHandler.phpto replace the scope on throw points extracted from the anonymous class constructor's statement result with the outer scope (the scope at thenewexpression site)replaceScope()method tosrc/Analyser/InternalThrowPoint.phptests/PHPStan/Rules/Variables/DefinedVariableRuleTest.phpandtests/PHPStan/Rules/Variables/data/bug-13920.phpRoot cause
When processing
new class() extends Foo { public function __construct() { parent::__construct(); } }, theNewHandlerextracted throw points from the constructor's body analysis. These throw points carried scopes from within the constructor method, which naturally didn't contain the outer method's variables. When the try/finally handling merged these throw point scopes into thefinallyScopeviamergeWith(), any outer variable present in the finally scope but absent from the inner constructor scope was downgraded from "definitely defined" (Yescertainty) to "might not be defined" (Maybecertainty).The fix replaces the inner scope on these throw points with the outer
$scopeat the call site, matching the behavior already used for inherited constructors (theelsebranch at line 196, which usesgetConstructorThrowPoint()with the outer scope).Test
Added a regression test that verifies no errors are reported when a variable is defined before a
tryblock containing an anonymous class withparent::__construct(), and that variable is used in thefinallyblock.Fixes phpstan/phpstan#13920